Ensure that the download modal is announced to screen reader users #3593
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
NickColley
force-pushed
the
improve-modal
branch
4 times, most recently
from
88c8463 to
dd6882c
Compare
Adds a lot of good features that make it way more accessible like preventing outside elements from being focused, which can be difficult to do otherwise. Removes dependency on leanmodel2 so quicker page loads. TODO before ready: - Fallback gracefully in browsers that dont support it - Test in real screen readers
NickColley
force-pushed
the
improve-modal
branch
from
dd6882c to
67846d9
Compare
NickColley
commented
Jul 24, 2024
| <div id="download-modal" class="dialog modal"> | ||
| <img alt="Download elementary OS icon" src="images/icons/apps/48/system-os-installer.svg"> | ||
| <dialog id="download-modal" class="dialog" aria-labelledby="download-modal-title"> | ||
| <img src="images/icons/apps/48/system-os-installer.svg" alt=""/> |
There was a problem hiding this comment.
I have set the alt to nothing to mark the image as presentational as it is just for show.
NickColley
commented
Jul 24, 2024
| @@ -467,21 +467,21 @@ | |||
|
|
|||
| <span id="translate-download" style="display:none;" hidden>Download elementary OS</span> | |||
| <span id="translate-purchase" style="display:none;" hidden>Purchase elementary OS</span> | |||
| <div id="download-modal" class="dialog modal"> | |||
| <img alt="Download elementary OS icon" src="images/icons/apps/48/system-os-installer.svg"> | |||
| <dialog id="download-modal" class="dialog" aria-labelledby="download-modal-title"> | |||
There was a problem hiding this comment.
Here I label the dialog based on the heading so that the dialog has a clear name "Choose a Download modal dialog" etc
NickColley
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I did consider improving the current implementation, I think adding focus management would have been fine but keyboard trapping is non-trivial so that's why adopting this is worthwhile as we get all that for free.
Resolves #3378
Browser support strategy
For browsers that do not support the native dialog we aim to show them this at the bottom of the page:
Which'll be linked to using regular anchor jumping.
For users that support the dialog element it'll look the same as it currently does.
I have not been able to test what happens when someone donates as I cant get stripe working locally but it should work since I've just changed the internal modal and not the place it gets called.
Screen reader testing
NVDA
Using https://assistivlabs.com/.
When I open the dialog I get this announced:
Browser testing
Note the modal icon is not shown in the screenshots because I cant get it to load for some reason, but it is unchanged.